Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix follow path checking at depths greater than 2 #8819

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

RealityAnomaly
Copy link
Member

@RealityAnomaly RealityAnomaly commented Aug 13, 2023

Motivation

Currently, you can't have follows paths with depths greater than 2 - you get an error along the lines of error: input 'B/D' follows a non-existent input 'B/C/D'.

Context

We need to recurse into the input tree to handle follows paths that trarverse multiple inputs that may or may not be follow paths themselves. I've added tests for the problem.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Aug 13, 2023
Copy link
Contributor

@iFreilicht iFreilicht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cherry-picked the test onto current master and it fails indeed with error: input 'B/D' follows a non-existent input 'B/C/D'. I then tested the full branch locally and it does indeed fix the test.

Code change itself is trivial, this can be merged.

If you want, you can also add a bullet to rl-next.md so the fix shows up in the next release notes, but it's not a requirement :)

@RealityAnomaly
Copy link
Member Author

If you want, you can also add a bullet to rl-next.md so the fix shows up in the next release notes, but it's not a requirement :)

Sure - I've added a small note.

We need to recurse into the input tree to handle follows paths that
trarverse multiple inputs that may or may not be follow paths
themselves.
Copy link
Contributor

@iFreilicht iFreilicht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just to set your expectations, I don't have commit access, so this PR might stay open until one of the maintainers gets around to merging it, despite my approval.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tricky one. I think the diagram I wrote in the suggestion could be helpful. Does it match your understanding of the problem?

@tomberek I think you mentioned a problem like this some time ago.

tests/flakes/follow-paths.sh Show resolved Hide resolved
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@tomberek
Copy link
Contributor

@tomberek I think you mentioned a problem like this some time ago.

The example I've been using: #5790 (comment)

Copy link
Contributor

@aakropotkin aakropotkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this PR @VertexA115 <3

tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
tests/flakes/follow-paths.sh Outdated Show resolved Hide resolved
Co-authored-by: Alex Ameen <alex.ameen.tx@gmail.com>
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would perhaps be nice to (also?) have a test that's based on the actual contents of the overridden flakes; maybe something from #5790?
Nonetheless the new test seems sufficient as a regression test.

@iFreilicht
Copy link
Contributor

The test on macos is failing only due to a flaky test. Could someone with CI access retrigger it?

@tomberek
Copy link
Contributor

@tomberek I think you mentioned a problem like this some time ago.

The example I've been using: #5790 (comment)

Note. This PR is solving a different problem than what I mentioned above.

@tomberek tomberek merged commit b563ef3 into NixOS:master Aug 25, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants